Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-115999: Add free-threaded specialization for CONTAINS_OP #126450

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Nov 5, 2024

@corona10
Copy link
Member Author

corona10 commented Nov 5, 2024

I just followed up on the change #126410. The specialization mechanism of this opcode is the same with COMPARE_OP IIUC, so it should be added too.

Copy link
Contributor

@mpage mpage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Would you please add a short explanation to either the pull request description or commit message outlining why the specialization logic + specialized instructions are thread-safe? I think what's here is correct, but it would be helpful for someone unfamiliar with this code, or if it turns out that our assumptions are invalid.

In this case, I think something like the following would be fine:

  • The specialization logic determines the appropriate specialization using only the operand's type, which is safe to read non-atomically (changing it requires stopping the world). We are guaranteed that the type will not change in between when it is checked and when we specialize the bytecode because the types involved are immutable (you cannot assign to __class__ for exact instances of dict, set, or frozenset). The bytecode is mutated atomically using helpers.
  • The specialized instructions rely on the operand type not changing in between the DEOPT_IF checks and the calls to the appropriate type-specific helpers (e.g. _PySet_Contains). This is a correctness requirement in the default builds and there are no changes to the opcodes in the free-threaded builds that would invalidate this.

@corona10 corona10 enabled auto-merge (squash) November 6, 2024 03:09
@corona10 corona10 merged commit 4ea214e into python:main Nov 6, 2024
55 checks passed
@corona10 corona10 deleted the gh-115999-contain-op branch November 6, 2024 03:37
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In future, can you please leave sufficient time to review PRs before merging.
Less than 24 hours is not enough. This PR was opened 5pm and merged at 4am UK time.

@@ -1335,6 +1335,27 @@ def test_call_specialize(self):
got = self.get_disassembly(co, adaptive=True)
self.do_disassembly_compare(got, call_quicken)

@cpython_only
Copy link
Member

@markshannon markshannon Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a test for the dis module. It should be moved to the appropriate place, probably test_opcache.

Also, please make this a behavioral test.
In other words, tests that the behavior is the same before and after specialization.

@corona10
Copy link
Member Author

corona10 commented Nov 6, 2024

In future, can you please leave sufficient time to review PRs before merging.
Less than 24 hours is not enough. This PR was opened 5pm and merged at 4am UK time.

Ah, sorry, I was too hurry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants